-
Notifications
You must be signed in to change notification settings - Fork 407
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Split recompile and writeback in transaction #6959
Conversation
d686440
to
9befc56
Compare
edb/server/dbview/dbview.pyx
Outdated
# * Issued a DDL or committing a tx with DDL (recompilation | ||
# before in-tx DDL needs to fix _in_tx_with_ddl caching 1st) | ||
# * Config.auto_rebuild_query_cache is turned on | ||
# * TODO: Modified `Config.apply_access_policies`? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think doing it when setting apply_access_policies
is a good idea.
Probably it will just be set for one or two specific queries, and the right thing is to just have it as part of the query cache key
@@ -1124,6 +1080,42 @@ cdef class DatabaseConnectionView: | |||
if not lock._waiters: | |||
del lock_table[query_req] | |||
|
|||
recompiled_cache = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this logic go in a separate function? (Fine if you don't think so)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was thinking about having all this decided in server/compiler/compiler.py
or deeper where we can have per-statement "dbview" of session_config, database_config and all (it's like what dbview.pyx
does in start()
/on_success()
, but it'll be too late for recompilation), so that we can handle scripts like:
configure session set auto_rebuild_query_cache := false;
create type Foo;
configure session set auto_rebuild_query_cache := true;
or use the correct config to recompile after scripts like:
configure current database set apply_access_policies := true;
create Type Foo;
So basically this logic is temporarily here and I'm expecting it to be replaced by a simple if query_unit_group.should_recompile
or something like that in the near future.
This reverts commit 9befc56bc1c285e4eba850b48b50bd38e4f01643.
This extracted the actual query cache recompilation to the time we compile the DDL itself, assuming the compiled DDL will soon execute with the recompiled cache updated in the same transaction.
We currently skip the recompilation for DDLs in a transaction and let the future COMMIT do it. The recompilation may unnecessarily block the DDL transaction (locking tables) for a long time because it's done before the COMMIT after DDLs. This shall be changed and improved after we add back the per-dbview cache for queries
_in_tx_with_ddl
, in a way that we recompile before applying the DDL in the database (locking tables) and hope there is no more DDLs in the same transaction.